Add new options like INHERIT and SET to the Role's membership tab. #6451 #9539
Conversation
WalkthroughRefactors role membership handling to use list-based structures, adds PostgreSQL 16+ support for INHERIT and SET options, updates UI schema and formatters for version gating, introduces new 16_plus SQL templates for role create/update/properties/sql, and expands tests to cover membership variants and special-character role names. Changes
Sequence Diagram(s)sequenceDiagram
participant User as UI (Client)
participant Frontend as SchemaView / Membership UI
participant Backend as pgAdmin Server
participant Templates as SQL Templating
participant DB as PostgreSQL
User->>Frontend: Open Role dialog / edit membership
Frontend->>Frontend: Use node_info.version to show fields (admin/inherit/set)
User->>Frontend: Add/modify membership entries
Frontend->>Backend: Submit role payload (rolmembership_list / rol_members_list)
Backend->>Backend: transform/validate payload (version-gated)
Backend->>Templates: Render CREATE/ALTER SQL (16_plus or default)
Templates->>DB: SQL executed / displayed to user
DB-->>Backend: Execution result / metadata
Backend-->>Frontend: Response (success/errors)
Frontend-->>User: Show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql (1)
33-38: Password NULL branch is unreachable with current outer condition.Outer guard is
if data.rolpassword(false whenNone), but the body checksdata.rolpassword is none(never true). If you intend to support explicitPASSWORD NULLon create, the outer condition should be based on key presence.Proposed fix
- VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if data.rolpassword %} + VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if 'rolpassword' in data %} PASSWORD {% if data.rolpassword is none %}NULL{% else %}{% if dummy %}'xxxxxx'{% else %} {{ data.rolpassword|qtLiteral(conn) }}{% endif %}{% endif %}{% endif %};web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
204-236: Avoid aliasing/mutating request payload lists in_process_rolemembership
In create mode (Line 219) you aliasrolmembershipintorolmembership_list, and in update mode youextend()a list pulled from the request (Line 221-233). This can cause surprising side effects if later code still reads the original structure.Proposed adjustment (defensive copy)
if id == -1: - data['rolmembership_list'] = data['rolmembership'] + data['rolmembership_list'] = list(data['rolmembership']) else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', [])) @@ - data['rolmembership_list'].extend( - data['rolmembership'].get('changed', [])) + data['rolmembership_list'].extend( + data['rolmembership'].get('changed', []))
270-348: Fix API contract: Documentation shows'updated'but code expects'changed'for role member updatesThe update-mode message (line 326+) documents an
'updated'key, but_validate_input_dict_for_update(line 155) and_process_rolmembers(lines 258–265) expect'changed'. Tests and all template code use'changed'. Any API client following the documented format will silently fail validation.Additionally, for PostgreSQL < 16, both the create and update messages (lines 287–289, 313–315) document
inheritandsetfields, but validation only requires['role', 'admin'](lines 292–293, 337–338), creating a misleading contract.Suggestion: Change the documented message keys from
'updated'to'changed'to match the actual implementation.
🤖 Fix all issues with AI agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py`:
- Around line 237-269: The code assigns lists from data['rolmembers'] directly
(e.g., in _process_rolmembers setting data['rol_members_list'] =
data['rolmembers'].get('added', []) and similar for changed/deleted) which
creates aliases that are later mutated with .extend(); make defensive copies
instead by wrapping those .get(...) results with list(...) so mutations do not
modify the original input structures, and apply the same defensive-copy change
in the analogous function _process_rolemembership for its list assignments.
In `@web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js`:
- Around line 61-66: The code building the badges inside rawData.forEach uses
member.admin, member.inherit and member.set directly (when computing badges for
serverVersion >= 160000) which can throw if those properties are undefined;
update the badge construction in the rawData.forEach callback (the block using
serverVersion, member.admin, member.inherit, member.set and appending to
members) to defensively coerce missing values to safe defaults (e.g., use
(member.inherit ?? false).toString().toUpperCase(), (member.set ??
false).toString().toUpperCase(), and similar for admin, and ensure member.role
is present before concatenation) so undefined/null won’t cause .toString() to
throw and concatenation into members is safe.
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql`:
- Around line 33-38: The template has an unreachable branch because it first
tests `{% if data.rolpassword %}` (truthy) and inside checks `{% if
data.rolpassword is none %}` (never hit); change the outer check to a
key-presence/definition test (e.g., `{% if data.rolpassword is defined %}`) so
the inner `is none` branch can run when the key exists but is null, ensuring the
`PASSWORD NULL` path is reachable; update the `PASSWORD` block surrounding
`data.rolpassword|qtLiteral(conn)` accordingly.
- Around line 38-43: The guard that checks for any of 'admin'/'inherit'/'set' in
data.rolmembership_list is fine, but the template then accesses item.admin,
item.inherit and item.set unguarded; update the WITH clause in the roles SQL
template to use safe defaults so missing keys don't break rendering—change the
clause to use Jinja's default filter for each key (e.g. ADMIN {{
item.admin|default(false) }}, INHERIT {{ item.inherit|default(false) }}, SET {{
item.set|default(false) }}), and apply the same change for the second occurrence
handling lines 57-62 so both GRANT renderings reference
item.admin|default(false), item.inherit|default(false) and
item.set|default(false).
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sql`:
- Around line 16-31: The rolmembers ARRAY SELECT is nondeterministic and uses
unqualified catalog names; update the subquery that builds rolmembers (the
SELECT returning CASE...||pg.usename from the derived table aliased pg) to
qualify system tables/functions with pg_catalog (e.g., pg_catalog.pg_roles,
pg_catalog.pg_auth_members) and add a deterministic ORDER BY (for example by
pg.usename and the option bits) in the outer SELECT so the ARRAY(...) produces
stable, repeatable output when rid is provided.
- Around line 1-15: The rolmembership array can contain NULL entries when
rm.rolname is NULL and its ORDER is nondeterministic; change the rolmembership
subselect to exclude NULL role names (e.g., add "WHERE rm.rolname IS NOT NULL"
or filter on rm.oid IS NOT NULL) and add an ORDER BY rm.rolname to the
ARRAY(...) SELECT to guarantee deterministic ordering; for seclabels, make the
array_agg deterministic by adding an ORDER BY inside array_agg, e.g.,
"array_agg(provider || '=' || label ORDER BY provider, label)" so results are
stable across runs.
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql`:
- Around line 3-10: In update.sql replace the implicit truthiness check "{% if
data.rolvaliduntil %}" with the explicit null check used in create.sql (e.g.,
"{% if data.rolvaliduntil and data.rolvaliduntil is not none %}" or at minimum
"{% if data.rolvaliduntil is not none %}") so that an empty string does not get
treated as a valid value; apply the same change to both the 16_plus and default
update.sql templates to mirror create.sql and ensure rolvaliduntil is handled
consistently.
- Around line 50-61: The template guard allows rendering if any of
'admin','inherit','set' exist but then unconditionally outputs all three, which
can produce broken SQL; update the roles SQL template so it either (a) requires
all three keys present in the condition (replace "if 'admin' in item or
'inherit' in item or 'set' in item" with an AND check) or (b) render each clause
only when its key exists (use per-key checks or Jinja default filters for
item.admin, item.inherit, item.set) so GRANT lines only include valid "WITH
ADMIN ..., INHERIT ..., SET ..." fragments for the current item (refer to the
template variables item, rolname, conn and the GRANT/REVOKE blocks).
In
`@web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql`:
- Around line 16-17: The expected output in alter_role_options4.sql is missing
two GRANT statements corresponding to the rolmembers "added" actions: add GRANT
for test_rolemembership_3 that grants "Role2_$%{}[]()&*^!@""'`\/#" TO
test_rolemembership_3 WITH ADMIN OPTION, INHERIT OPTION, SET OPTION, and add
GRANT for test_rolemembership_4 that grants "Role2_$%{}[]()&*^!@""'`\/#" TO
test_rolemembership_4; insert these two GRANT statements alongside the existing
GRANT lines for test_rolemembership_2 and test_rolemembership_1 so the expected
output matches the alter_role_options4.msql template.
In `@web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js`:
- Around line 76-80: The depChange handler currently forces state.set = false
when undefined; update the depChange for the "set" field (function depChange) to
either leave state.set undefined or initialize it to true to match the
PostgreSQL 16+ default (i.e., change the fallback from false to true or remove
the override altogether) so new rows inherit the intended default; ensure you
only touch depChange and keep the rest of the field logic intact.
🧹 Nitpick comments (7)
web/pgadmin/static/js/SchemaView/common.js (1)
19-31: Improve readability and add defensive check forhelperProps.The proposed refactor is recommended:
versionis correctly typed asPropTypes.number(safe for comparisons), but the code lacks a guard forhelperPropsitself before accessinghelperProps.serverInfo. While PropTypes enforcement currently ensureshelperPropsis provided, adding an explicit check makes the function more robust and improves readability through sequential returns instead of nested ternaries.Proposed refactor
export const isVersionSupportedByField = (field, helperProps) => { - return ( - // serverInfo not found - _.isUndefined(helperProps.serverInfo) || - // serverInfo found and it's within range - ( - _.isUndefined(field.min_version) ? true : - (helperProps.serverInfo.version >= field.min_version) - ) && ( - _.isUndefined(field.max_version) ? true : - (helperProps.serverInfo.version <= field.max_version) - )); + if (_.isUndefined(helperProps) || _.isUndefined(helperProps.serverInfo)) return true; + const v = helperProps.serverInfo.version; + if (!_.isUndefined(field.min_version) && v < field.min_version) return false; + if (!_.isUndefined(field.max_version) && v > field.max_version) return false; + return true; };docs/en_US/role_dialog.rst (1)
77-87: Documentation inconsistency: Inherit and Set options not mentioned in section descriptions.Lines 77-81 describe using "Admin Option" checkboxes for both sections, but the note on line 87 mentions that "Inherit Option" and "Set Option" are also available for PostgreSQL 16+. Consider updating lines 77-81 to mention that additional options (Inherit, Set) are available as checkboxes when using PostgreSQL 16 or later.
📝 Suggested documentation improvement
-* Use *Member of* section to specify roles of which the current role - is a member. To assign *Admin Option* for a selected role, click on - the appropriate checkbox. -* Use *Members* section to specify roles that are members of the current - role. To assign *Admin Option* for a selected role, click on the appropriate checkbox. +* Use *Member of* section to specify roles of which the current role + is a member. To assign options for a selected role, click on + the appropriate checkboxes (*Admin*, *Inherit*, *Set*). +* Use *Members* section to specify roles that are members of the current + role. To assign options for a selected role, click on the appropriate checkboxes.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msql (1)
5-5: Minor: Extra whitespace beforeWITH.There's a double space between the role name and
WITH ADMIN. While this doesn't affect functionality, it's inconsistent with the other lines.📝 Suggested fix
-GRANT "Role2_$%{}[]()&*^!@""'`\/#" TO test_rolemembership_3 WITH ADMIN False, INHERIT False, SET False; +GRANT "Role2_$%{}[]()&*^!@""'`\/#" TO test_rolemembership_3 WITH ADMIN False, INHERIT False, SET False;web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.json (1)
156-209: Regex replacement for VALID UNTIL may be brittle across timestamp formatting changes.If the SQL generator changes spacing/timezone rendering,
VALID UNTIL '[0-9\\-T:+ ]*'might stop matching. Consider constraining less to formatting specifics (or matching onVALID UNTIL '.*'if your harness is safe against overmatching).web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql (1)
108-119: The template condition-access mismatch can be made more defensive, but validation guarantees key existence for PG16+.The GRANT clause at line 117 uses OR logic to check for keys (
'admin' in item or 'inherit' in item or 'set' in item) but unconditionally accesses all three ({{ item.admin }}, {{ item.inherit }}, {{ item.set }}). While this appears unsafe, PG16+ validation (lines 294-300, 336-338) strictly requires all four keys including 'role', 'admin', 'inherit', 'set' in each item before the template is rendered—so the pattern cannot fail at runtime.However, adding defensive defaults is reasonable for template robustness. A similar issue exists at line 59 in the
rolmembership_listGRANT clause.Suggested fix for line 117
{% if data.rol_members_list and data.rol_members_list|length > 0 %} {% for item in data.rol_members_list %} -GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }} {% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; +{% set admin_opt = item.admin|default(false) %} +{% set inherit_opt = item.inherit|default(false) %} +{% set set_opt = item.set|default(false) %} +GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ admin_opt }}, INHERIT {{ inherit_opt }}, SET {{ set_opt }}{% endif %}; {% endfor %} {% endif %}web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
349-427: Sameupdatedvschangedmismatch in_validate_rolemembership(+ PG<16 message mentions inherit/set)
Same issue as above: the update-mode message documentsupdated(Line 405+) but backend expects'changed'. Also, messages includeinherit/setfor PG<16 even though they’re not required there.
842-865: Reduce duplication + add same regex guard intransform()rolemembership parsing
transform()duplicates the same parsing logic as_set_rolemembers()(Line 848-861) and has the same “regex can be None” crash risk. Recommend extracting a small helper to parse the flag-prefixed role strings (for both rolmembership and rolmembers) and reusing it from both places.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/en_US/images/role_membership.pngis excluded by!**/*.pngdocs/en_US/images/role_sql.pngis excluded by!**/*.png
📒 Files selected for processing (41)
docs/en_US/role_dialog.rstweb/pgadmin/browser/server_groups/servers/roles/__init__.pyweb/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.jsweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.jsonweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/test.jsonweb/pgadmin/browser/server_groups/servers/static/js/membership.ui.jsweb/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsxweb/pgadmin/static/js/SchemaView/common.js
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsx (1)
web/pgadmin/static/js/SchemaView/common.js (2)
isVersionSupportedByField(19-31)isVersionSupportedByField(19-31)
web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js (1)
web/pgadmin/static/js/SchemaView/base_schema.ui.js (1)
BaseUISchema(17-212)
🔇 Additional comments (39)
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msql (1)
1-9: LGTM!The CREATE ROLE statement correctly escapes the complex role name with special characters and sets appropriate privileges for a login role test case. The SQL syntax is valid for PostgreSQL 16+.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msql (1)
1-1: LGTM!The COMMENT ON ROLE statement is syntactically correct with proper identifier escaping matching the corresponding role creation test.
web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msql (1)
1-3: LGTM!The REVOKE statements correctly test both membership directions - revoking a role from the complex-named role and revoking the complex-named role from another role. The identifier escaping is consistent.
web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sql (1)
16-17: LGTM!The GRANT statements are correctly split into individual statements, allowing granular control over the ADMIN OPTION per membership. This aligns with the PR's objective to support enhanced role membership options.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sql (1)
1-13: LGTM!The SQL script correctly represents a PostgreSQL 16+ role with LOGIN privileges and comprehensive attributes. The commented DROP ROLE follows the standard pattern for generated SQL output, and the COMMENT ON ROLE provides metadata for the test scenario.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sql (1)
1-2: Special-character role identifier quoting looks correct (embedded"doubled as""; other characters safely contained within quoted identifier).Also applies to: 13-13
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sql (2)
16-18: DB-scoped setting + role comment statements look good and well-terminated.
4-15: CREATE statement in an ALTER-type test fixture is semantically inconsistent.This file is referenced in a test marked as
"type": "alter"for the operation "Alter Login Role options", yet the expected SQL begins withCREATE ROLErather thanALTER ROLE. While the file also contains anALTER ROLE ... IN DATABASEstatement on line 16 and aCOMMENT ON ROLEon line 18, the leadingCREATEstatement is unusual for a fixture documenting an alter operation.If this fixture represents the actual SQL statements to be executed for the ALTER operation, it should start with
ALTER ROLE. If it instead represents the role's final state representation, the filename should reflect that (e.g.,alter_login_role_options_final_state.sql).Suggested change (if this should show actual ALTER statements)
-CREATE ROLE "Role2_$%{}[]()&*^!@""'`\/#" WITH +ALTER ROLE "Role2_$%{}[]()&*^!@""'`\/#" WITH LOGIN NOSUPERUSER INHERIT NOCREATEDB CREATEROLE NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 100 ENCRYPTED PASSWORD '<PASSWORD>' VALID UNTIL '<TIMESTAMPTZ_1>';web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msql (1)
12-19: Ensure expected output fixtures were updated to match the new SQL statements.The GRANT and ALTER ROLE statements added (lines 12-19) are syntactically valid, but this test file change requires corresponding updates to expected-output files (likely
.sqlor.jsonfixtures in the same test directory). Without confirmation that test expectations were updated to reflect these new statements, the test may fail or not properly validate the altered behavior.Verify that:
- Expected output fixtures for
alter_role_options1test scenario have been updated- Test framework expectations align with the added GRANT statements and ALTER ROLE configuration
web/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsx (1)
11-12: Version-gated column visibility logic is correct. ThecolumnVisibilityfield properly ANDs both mode and version support checks on lines 50–52. WhenserverInfois absent,isVersionSupportedByFieldreturnstrue, allowing version-gated fields to remain visible—which is the intended fallback behavior.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sql (1)
1-13: LGTM!The test file correctly demonstrates PostgreSQL role creation with a complex special-character identifier and proper comment attachment. The double-quote escaping (
"") within the identifier is correctly applied in both theCREATE ROLEandCOMMENT ON ROLEstatements.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msql (1)
1-1: LGTM!The
.msqlfile correctly isolates theCOMMENT ON ROLEstatement for testing the role description modification, with proper identifier escaping matching the companion.sqlfile.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sql (1)
1-11: LGTM!The test file correctly defines a login role with full privileges using proper identifier escaping. The commented
DROP ROLE IF EXISTSprovides a useful cleanup reference for manual testing.web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msql (1)
1-7: LGTM!The test correctly uses pre-PostgreSQL 16 syntax for role membership alterations, with
GRANT ... WITH ADMIN OPTIONandREVOKE ADMIN OPTION FORstatements. TheINHERITandSEToptions are appropriately omitted since they are only available in PostgreSQL 16+, and this is in thedefaulttest directory.web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msql (1)
1-7: LGTM!The test correctly exercises bidirectional role membership modifications with
ADMIN OPTIONgrants and revokes, using appropriate pre-PostgreSQL 16 syntax. The proper identifier escaping for the special-character role name is maintained throughout.web/pgadmin/browser/server_groups/servers/roles/tests/default/test.json (2)
45-86: LGTM!The additional test roles (3 and 4) provide proper coverage for testing membership scenarios with multiple roles.
148-149: LGTM!The test scenarios comprehensively cover membership operations (add, change, delete, and combined) for both
rolmembershipandrolmembers. The delete scenario correctly includes all four test role IDs.Also applies to: 163-164, 179-180, 197-202, 220-220
web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js (2)
24-30: LGTM on version-gated defaults.The constructor properly gates
adminandsetdefaults based on server version, and theinheritfield is correctly added withundefineddefault (to be set via depChange).
61-70: LGTM on theinheritfield.The field is properly gated with
min_version: 160000and thedepChangecorrectly initializes undefined values tofalse.web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql (2)
31-56: LGTM!The GRANT statement generation correctly handles the PostgreSQL 16+ membership options (admin, inherit, set) with proper conditional formatting. The grouping by option flags efficiently batches grants with identical option sets.
1-29: LGTM!The CREATE ROLE statement template properly generates all role attributes including the PostgreSQL 16+ BYPASSRLS option and handles password encryption detection correctly.
web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js (1)
180-183: LGTM!Wrapping in an arrow function
(raw) => obj.memberDataFormatter(raw)ensures properthisbinding when the formatter is invoked as a callback.Also applies to: 201-204
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sql (1)
1-11: LGTM!The SQL test fixture correctly demonstrates PostgreSQL 16+ role creation with proper identifier quoting. The special characters in the role name are appropriately escaped (doubled double-quotes for embedded quotes), and the role attributes follow standard PostgreSQL syntax.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msql (1)
1-9: LGTM!The SQL test fixture is valid and properly includes
CONNECTION LIMIT -1(unlimited connections). The attribute ordering differs slightly fromcreate_role.sql, which is acceptable as these likely test different template code paths.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sql (1)
1-13: LGTM!The test fixture correctly demonstrates a role with proper identifier escaping and includes a
COMMENT ON ROLEstatement. This appears to represent the expected SQL output for the role after a rename operation in the test workflow.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msql (1)
1-2: LGTM!The
ALTER ROLE ... RENAME TOstatement correctly demonstrates role renaming with properly escaped special characters in both the source and target role names.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msql (1)
1-3: LGTM!The test fixture correctly demonstrates bidirectional role membership revocation. The first statement revokes
test_rolemembership_1from the complex-named role (removing its membership in that role), while the second revokes the complex-named role fromtest_rolemembership_3. Proper identifier quoting is maintained throughout.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msql (1)
1-2: LGTM!The ALTER ROLE RENAME statement correctly handles the complex identifier with proper double-quote escaping. The SQL syntax is valid for PostgreSQL.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sql (1)
1-21: LGTM!The SQL test file correctly demonstrates PostgreSQL 16+ role creation with:
- Proper role options (SUPERUSER, CREATEDB, etc.)
- Correct GRANT syntax using
WITH ADMIN OPTION, INHERIT OPTION, SET OPTION- Database-scoped configuration via
ALTER ROLE IN DATABASE- Appropriate use of placeholders for sensitive/variable data
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msql (1)
1-12: LGTM!The ALTER ROLE statements are syntactically valid. The ISO 8601 timestamp format with 'T' separator is accepted by PostgreSQL.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql (1)
1-21: Approve overall structure and syntax.The CREATE ROLE statement and supporting SQL are syntactically correct for PostgreSQL 16+. The file properly demonstrates role creation with membership grants using the
WITH ... OPTIONsyntax.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msql (1)
1-19: LGTM - Comprehensive test coverage for PostgreSQL 16+ role membership options.The test correctly exercises:
- ALTER ROLE with various attributes
- GRANT statements in both directions (roles granted to this role, and this role granted to others)
- All three new PostgreSQL 16+ options (ADMIN, INHERIT, SET) with both True and False values
- Proper escaping of special characters in the role name
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sql (1)
1-20: LGTM - Expected state file correctly shows PostgreSQL 16+ GRANT syntax.The file uses the standard
WITH ADMIN OPTION, INHERIT OPTION, SET OPTIONsyntax for the remaining granted membership, which is appropriate for expected-state test files.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sql (1)
16-17: LGTM - Correctly shows differentiated GRANT syntax.Line 16 omits options when all are false, while line 17 includes all options when true. This matches the expected behavior where options are only included in the SQL output when enabled.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msql (1)
1-7: Test file correctly covers all PostgreSQL 16+ membership option combinations.The file tests GRANT statements in both directions with both True and False values for all three options (ADMIN, INHERIT, SET).
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql (2)
50-69: LGTM - Role membership handling correctly refactored to list-based structure.The template correctly:
- Iterates over
rolmembership_revoked_listfor REVOKE statements- Iterates over
rolmembership_listfor GRANT statements with optional ADMIN OPTION- Handles
rolmembership_revoked_adminsfor revoking admin privilegesThe inner condition check on lines 64-67 filters items during iteration, which is functionally correct.
116-135: LGTM - Symmetric handling for the reverse membership direction.The template correctly mirrors the
rolmembership_*logic forrol_members_*lists, handling:
- Revoking this role from other members
- Granting this role to other members with optional ADMIN OPTION
- Revoking admin option from specific members
This maintains consistency between both membership directions.
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql (1)
38-43: List-driven membership grants look good for pre-16 behavior (ADMIN OPTION only).Also applies to: 57-62
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.json (1)
1-299: Test payload transformation is correctly implemented and properly version-gated.The transformation layer in
roles/__init__.pycorrectly maps the test payloads:
_process_rolemembership()transformsrolmembership.{added,changed,deleted}→rolmembership_list+rolmembership_revoked_list_process_rolmembers()transformsrolmembers.{added,changed,deleted}→rol_members_list+rol_members_revoked_listBoth methods include proper version gating (< 160000 vs >= 160000) to handle the additional
inheritandsetproperties in PostgreSQL 16+. The test payloads use the correct public API shape and will be properly transformed before template rendering.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def _process_rolmembers(self, id, data): | ||
| """ | ||
| Parser role members. | ||
| :param id: | ||
| :param data: | ||
| """ | ||
| def _part_dict_list(dict_list, condition, list_key=None): | ||
| def _part_dict_list(dict_list, condition): | ||
| ret_val = [] | ||
| for d in dict_list: | ||
| if condition(d): | ||
| ret_val.append(d[list_key]) | ||
| ret_val.append(d) | ||
|
|
||
| return ret_val | ||
| if id == -1: | ||
| data['rol_members'] = [] | ||
| data['rol_admins'] = [] | ||
| data['rol_members_list'] = data['rolmembers'] | ||
|
|
||
| data['rol_admins'] = _part_dict_list( | ||
| data['rolmembers'], lambda d: d['admin'], 'role') | ||
| data['rol_members'] = _part_dict_list( | ||
| data['rolmembers'], lambda d: not d['admin'], 'role') | ||
| else: | ||
| data['rol_admins'] = _part_dict_list( | ||
| data['rolmembers'].get('added', []), | ||
| lambda d: d['admin'], 'role') | ||
| data['rol_members'] = _part_dict_list( | ||
| data['rolmembers'].get('added', []), | ||
| lambda d: not d['admin'], 'role') | ||
|
|
||
| data['rol_admins'].extend(_part_dict_list( | ||
| data['rolmembers'].get('changed', []), | ||
| lambda d: d['admin'], 'role')) | ||
| data['rol_revoked_admins'] = _part_dict_list( | ||
| data['rolmembers'].get('changed', []), | ||
| lambda d: not d['admin'], 'role') | ||
|
|
||
| data['rol_revoked'] = _part_dict_list( | ||
| data['rolmembers'].get('deleted', []), | ||
| lambda _: True, 'role') | ||
| data['rol_members_list'] = data['rolmembers'].get('added', []) | ||
|
|
||
| if self.manager.version < 160000: | ||
| data['rol_members_list'].extend(_part_dict_list( | ||
| data['rolmembers'].get('changed', []), | ||
| lambda d: d['admin'])) | ||
| data['rol_members_revoked_admins'] = _part_dict_list( | ||
| data['rolmembers'].get('changed', []), | ||
| lambda d: not d['admin']) | ||
| else: | ||
| data['rol_members_list'].extend( | ||
| data['rolmembers'].get('changed', [])) | ||
|
|
||
| data['rol_members_revoked_list'] = ( | ||
| data['rolmembers'].get('deleted', [])) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --hidden --no-ignore-vcs -S \
"rolmembership_list|rolmembership_revoked_list|rolmembership_revoked_admins|rol_members_list|rol_members_revoked_list|rol_members_revoked_admins" .Repository: pgadmin-org/pgadmin4
Length of output: 5901
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '200,280p'Repository: pgadmin-org/pgadmin4
Length of output: 3434
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '1,150p'Repository: pgadmin-org/pgadmin4
Length of output: 5592
🏁 Script executed:
rg -n "_process_rolmembers|_process_rolemembership" web/pgadmin/browser/server_groups/servers/roles/__init__.py | head -20Repository: pgadmin-org/pgadmin4
Length of output: 369
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '295,360p'Repository: pgadmin-org/pgadmin4
Length of output: 2446
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '440,520p'Repository: pgadmin-org/pgadmin4
Length of output: 2473
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '520,600p'Repository: pgadmin-org/pgadmin4
Length of output: 3165
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '600,700p'Repository: pgadmin-org/pgadmin4
Length of output: 4366
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '700,800p'Repository: pgadmin-org/pgadmin4
Length of output: 4112
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '800,900p'Repository: pgadmin-org/pgadmin4
Length of output: 4440
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '900,1000p'Repository: pgadmin-org/pgadmin4
Length of output: 3851
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '1000,1100p'Repository: pgadmin-org/pgadmin4
Length of output: 4006
🏁 Script executed:
cat web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql | sed -n '35,65p'Repository: pgadmin-org/pgadmin4
Length of output: 1340
🏁 Script executed:
cat web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql | sed -n '50,130p'Repository: pgadmin-org/pgadmin4
Length of output: 3035
Consider defensive copy in list assignments, but naming is intentional
The aliasing concern at lines 251 and 254 is valid—assigning data['rolmembers'].get('added', []) directly creates a reference that gets mutated via .extend() at lines 257 and 264. A defensive copy with list() is reasonable for safety.
However, the naming inconsistency claim is inaccurate. The two patterns (rolmembership_* vs rol_members_*) are intentional and required by templates: they correspond to the input dictionary keys (data['rolmembership'] vs data['rolmembers']). Template usage confirms both patterns are correctly wired throughout create.sql and update.sql.
Proposed adjustment (defensive copy)
if id == -1:
- data['rol_members_list'] = data['rolmembers']
+ data['rol_members_list'] = list(data['rolmembers'])
@@
else:
- data['rol_members_list'] = data['rolmembers'].get('added', [])
+ data['rol_members_list'] = list(data['rolmembers'].get('added', []))Note: The same aliasing pattern exists in _process_rolemembership (lines 219, 221–232) and warrants the same treatment.
🤖 Prompt for AI Agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py` around lines 237
- 269, The code assigns lists from data['rolmembers'] directly (e.g., in
_process_rolmembers setting data['rol_members_list'] =
data['rolmembers'].get('added', []) and similar for changed/deleted) which
creates aliases that are later mutated with .extend(); make defensive copies
instead by wrapping those .get(...) results with list(...) so mutations do not
modify the original input structures, and apply the same defensive-copy change
in the analogous function _process_rolemembership for its list assignments.
| def _set_rolemembers(self, row): | ||
|
|
||
| if 'rolmembers' in row and row['rolmembers'] is not None: | ||
| rolmembers = [] | ||
| for role in row['rolmembers']: | ||
| role = re.search(r'([01])(.+)', role) | ||
| rolmembers.append({ | ||
| 'role': role.group(2), | ||
| 'admin': True if role.group(1) == '1' else False | ||
| }) | ||
| if self.manager.version < 160000: | ||
| role = re.search(r'([01])(.+)', role) | ||
| rolmembers.append({ | ||
| 'role': role.group(2), | ||
| 'admin': True if role.group(1) == '1' else False | ||
| }) | ||
| else: | ||
| role = re.search(r'([01])([01])([01])(.+)', role) | ||
| rolmembers.append({ | ||
| 'role': role.group(4), | ||
| 'admin': True if role.group(1) == '1' else False, | ||
| 'inherit': True if role.group(2) == '1' else False, | ||
| 'set': True if role.group(3) == '1' else False | ||
| }) | ||
| row['rolmembers'] = rolmembers | ||
|
|
There was a problem hiding this comment.
Harden _set_rolemembers parsing: re.search(...) can be None → AttributeError
If row['rolmembers'] contains any unexpected string (or empty), re.search(...) can return None and group() will crash (Line 827-839). Given this runs in properties/list flows, it’s worth hardening.
Proposed hardening (use anchored match + guard)
def _set_rolemembers(self, row):
@@
for role in row['rolmembers']:
if self.manager.version < 160000:
- role = re.search(r'([01])(.+)', role)
+ m = re.match(r'^([01])(.+)$', role or '')
+ if not m:
+ continue
rolmembers.append({
- 'role': role.group(2),
- 'admin': True if role.group(1) == '1' else False
+ 'role': m.group(2),
+ 'admin': m.group(1) == '1'
})
else:
- role = re.search(r'([01])([01])([01])(.+)', role)
+ m = re.match(r'^([01])([01])([01])(.+)$', role or '')
+ if not m:
+ continue
rolmembers.append({
- 'role': role.group(4),
- 'admin': True if role.group(1) == '1' else False,
- 'inherit': True if role.group(2) == '1' else False,
- 'set': True if role.group(3) == '1' else False
+ 'role': m.group(4),
+ 'admin': m.group(1) == '1',
+ 'inherit': m.group(2) == '1',
+ 'set': m.group(3) == '1'
})| const serverVersion = this.nodeInfo && this.nodeInfo.server && this.nodeInfo.server.version || 0; | ||
| rawData.forEach(member => { | ||
| let withAdmin = ''; | ||
| if(member.admin) { withAdmin = ' [WITH ADMIN]';} | ||
| let badges = serverVersion >= 160000 ? ` [WITH ADMIN ${member.admin.toString().toUpperCase()}, INHERIT ${member.inherit.toString().toUpperCase()}, SET ${member.set.toString().toUpperCase()}]` : member.admin ? ' [WITH ADMIN OPTION]' : ''; | ||
|
|
||
| if (members.length > 0) { members += ', '; } | ||
| members = members + (member.role + withAdmin); | ||
| members = members + (member.role + badges); |
There was a problem hiding this comment.
Add defensive checks for potentially undefined member properties.
For PostgreSQL 16+, the code accesses member.inherit and member.set directly. If these properties are undefined or null (e.g., from legacy data or API inconsistencies), calling .toString() will throw a runtime error.
🔧 Suggested defensive fix
- const serverVersion = this.nodeInfo && this.nodeInfo.server && this.nodeInfo.server.version || 0;
+ const serverVersion = this.nodeInfo?.server?.version || 0;
rawData.forEach(member => {
- let badges = serverVersion >= 160000 ? ` [WITH ADMIN ${member.admin.toString().toUpperCase()}, INHERIT ${member.inherit.toString().toUpperCase()}, SET ${member.set.toString().toUpperCase()}]` : member.admin ? ' [WITH ADMIN OPTION]' : '';
+ let badges = '';
+ if (serverVersion >= 160000) {
+ const admin = (member.admin ?? false).toString().toUpperCase();
+ const inherit = (member.inherit ?? false).toString().toUpperCase();
+ const set = (member.set ?? true).toString().toUpperCase();
+ badges = ` [WITH ADMIN ${admin}, INHERIT ${inherit}, SET ${set}]`;
+ } else {
+ badges = member.admin ? ' [WITH ADMIN OPTION]' : '';
+ }
if (members.length > 0) { members += ', '; }
members = members + (member.role + badges);🤖 Prompt for AI Agents
In `@web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js` around
lines 61 - 66, The code building the badges inside rawData.forEach uses
member.admin, member.inherit and member.set directly (when computing badges for
serverVersion >= 160000) which can throw if those properties are undefined;
update the badge construction in the rawData.forEach callback (the block using
serverVersion, member.admin, member.inherit, member.set and appending to
members) to defensively coerce missing values to safe defaults (e.g., use
(member.inherit ?? false).toString().toUpperCase(), (member.set ??
false).toString().toUpperCase(), and similar for admin, and ensure member.role
is present before concatenation) so undefined/null won’t cause .toString() to
throw and concatenation into members is safe.
| CONNECTION LIMIT {{ data.rolconnlimit }}{% endif %}{% if data.rolvaliduntil and data.rolvaliduntil is not none %} | ||
|
|
||
| VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if data.rolpassword %} | ||
|
|
||
| PASSWORD {% if data.rolpassword is none %}NULL{% else %}{% if dummy %}'xxxxxx'{% else %} {{ data.rolpassword|qtLiteral(conn) }}{% endif %}{% endif %}{% endif %}; | ||
| {% if data.rolmembership_list and data.rolmembership_list|length > 0 %} |
There was a problem hiding this comment.
Same unreachable PASSWORD NULL branch on create (consider key-presence check).
Proposed fix
- VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if data.rolpassword %}
+ VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if 'rolpassword' in data %}
PASSWORD {% if data.rolpassword is none %}NULL{% else %}{% if dummy %}'xxxxxx'{% else %} {{ data.rolpassword|qtLiteral(conn) }}{% endif %}{% endif %}{% endif %};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CONNECTION LIMIT {{ data.rolconnlimit }}{% endif %}{% if data.rolvaliduntil and data.rolvaliduntil is not none %} | |
| VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if data.rolpassword %} | |
| PASSWORD {% if data.rolpassword is none %}NULL{% else %}{% if dummy %}'xxxxxx'{% else %} {{ data.rolpassword|qtLiteral(conn) }}{% endif %}{% endif %}{% endif %}; | |
| {% if data.rolmembership_list and data.rolmembership_list|length > 0 %} | |
| CONNECTION LIMIT {{ data.rolconnlimit }}{% endif %}{% if data.rolvaliduntil and data.rolvaliduntil is not none %} | |
| VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if 'rolpassword' in data %} | |
| PASSWORD {% if data.rolpassword is none %}NULL{% else %}{% if dummy %}'xxxxxx'{% else %} {{ data.rolpassword|qtLiteral(conn) }}{% endif %}{% endif %}{% endif %}; | |
| {% if data.rolmembership_list and data.rolmembership_list|length > 0 %} |
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql`
around lines 33 - 38, The template has an unreachable branch because it first
tests `{% if data.rolpassword %}` (truthy) and inside checks `{% if
data.rolpassword is none %}` (never hit); change the outer check to a
key-presence/definition test (e.g., `{% if data.rolpassword is defined %}`) so
the inner `is none` branch can run when the key exists but is null, ensuring the
`PASSWORD NULL` path is reachable; update the `PASSWORD` block surrounding
`data.rolpassword|qtLiteral(conn)` accordingly.
| {% if data.rolmembership_list and data.rolmembership_list|length > 0 %} | ||
| {% for item in data.rolmembership_list %} | ||
|
|
||
| GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(data.rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; | ||
| {% endfor %} | ||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "create.sql" -path "*/16_plus/*" -type fRepository: pgadmin-org/pgadmin4
Length of output: 184
🏁 Script executed:
cat -n "web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql" | head -70Repository: pgadmin-org/pgadmin4
Length of output: 2844
🏁 Script executed:
cat -n "web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql" | head -70Repository: pgadmin-org/pgadmin4
Length of output: 3064
🌐 Web query:
PostgreSQL 16 GRANT role membership syntax ADMIN INHERIT SET options
💡 Result:
Summary — GRANT for role membership (PostgreSQL 16)
-
Syntax (role-membership form):
GRANT role_name [, ...] TO grantee_role [, ...] [ WITH ( ADMIN | INHERIT | SET ) { TRUE | FALSE } [, ... ] ] [ GRANTED BY grantor ];
Example:
GRANT analytics TO alice WITH INHERIT TRUE, SET TRUE, ADMIN FALSE;
(Documentation shows the exact SQL form and notes that OPTION is a synonym for TRUE.) [1] -
What the options do:
- WITH ADMIN (WITH ADMIN TRUE / WITH ADMIN OPTION)
- Allows the grantee to grant/revoke that role to/from others (admin option). Defaults to FALSE. [1]
- WITH INHERIT
- Controls automatic inheritance of the role’s privileges in a session. If TRUE the grantee automatically sees privileges of the role; if FALSE they do not unless they SET ROLE. Defaults to the grantee’s INHERIT attribute when granting; omission when ALTERing preserves current value. See role membership behavior examples. [2]
- WITH SET
- Allows the grantee to use SET ROLE to become the granted role (or an indirect role only if every link in the grant chain has SET TRUE). Defaults to TRUE. [1][2]
- WITH ADMIN (WITH ADMIN TRUE / WITH ADMIN OPTION)
-
Additional notes
- GRANTED BY records a grantor; the recorded grant depends on the grantor retaining ADMIN OPTION. [1]
- Memberships cannot be granted to PUBLIC and circular grants are disallowed. [2]
- CREATE/ALTER ROLE clauses (IN ROLE / ROLE / ADMIN) create initial memberships with specific defaults (see CREATE ROLE). [4]
Sources: PostgreSQL 16 documentation — GRANT (role-membership), Role Membership, SET ROLE, CREATE ROLE. [1][2][5][4]
References
[1] PostgreSQL 16 SQL GRANT (role-membership) documentation.
[2] PostgreSQL 16 — 22.3 Role Membership.
[4] PostgreSQL 16 — CREATE ROLE (IN/ROLE/ADMIN clauses).
[5] PostgreSQL 16 — SET ROLE.
Fix missing-key hazards in ADMIN/INHERIT/SET rendering.
The guard condition checks if any of the three keys exist ('admin' in item or 'inherit' in item or 'set' in item), but then unconditionally renders all three values in the WITH clause. If only one or two keys are present in the item dictionary, accessing the missing keys will cause undefined behavior in Jinja2 rendering.
PostgreSQL 16+ GRANT syntax requires boolean values for ADMIN, INHERIT, and SET options. Use safe defaults with the |default(false) filter to guard against missing keys:
Proposed fix
{% if data.rolmembership_list and data.rolmembership_list|length > 0 %}
{% for item in data.rolmembership_list %}
+{% set admin_opt = item.admin|default(false) %}
+{% set inherit_opt = item.inherit|default(false) %}
+{% set set_opt = item.set|default(false) %}
-GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(data.rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %};
+GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(data.rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ admin_opt }}, INHERIT {{ inherit_opt }}, SET {{ set_opt }}{% endif %};
{% endfor %}
{% endif %}
{% if data.rol_members_list and data.rol_members_list|length > 0 %}
{% for item in data.rol_members_list %}
+{% set admin_opt = item.admin|default(false) %}
+{% set inherit_opt = item.inherit|default(false) %}
+{% set set_opt = item.set|default(false) %}
-GRANT {{ conn|qtIdent(data.rolname) }} TO {{ conn|qtIdent(item.role) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %};
+GRANT {{ conn|qtIdent(data.rolname) }} TO {{ conn|qtIdent(item.role) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ admin_opt }}, INHERIT {{ inherit_opt }}, SET {{ set_opt }}{% endif %};
{% endfor %}
{% endif %}Applies to lines 38-43 and 57-62.
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql`
around lines 38 - 43, The guard that checks for any of 'admin'/'inherit'/'set'
in data.rolmembership_list is fine, but the template then accesses item.admin,
item.inherit and item.set unguarded; update the WITH clause in the roles SQL
template to use safe defaults so missing keys don't break rendering—change the
clause to use Jinja's default filter for each key (e.g. ADMIN {{
item.admin|default(false) }}, INHERIT {{ item.inherit|default(false) }}, SET {{
item.set|default(false) }}), and apply the same change for the second occurrence
handling lines 57-62 so both GRANT renderings reference
item.admin|default(false), item.inherit|default(false) and
item.set|default(false).
| {% if 'rolname' in data %} | ||
| {% set rolname=data.rolname %} | ||
| ALTER ROLE {{ conn|qtIdent(role) }} | ||
| RENAME TO {{ conn|qtIdent(rolname) }}; | ||
|
|
||
| {% else %} | ||
| {% set rolname=role %} | ||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if any caller can send rolvaliduntil="" and what behavior is expected.
rg -n --type=py --type=js -C2 "rolvaliduntil" web/pgadmin/browser/server_groups/servers/rolesRepository: pgadmin-org/pgadmin4
Length of output: 184
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sqlRepository: pgadmin-org/pgadmin4
Length of output: 5184
🏁 Script executed:
find web/pgadmin/browser/server_groups/servers/roles/templates -name "*.sql" -type f | head -10Repository: pgadmin-org/pgadmin4
Length of output: 950
🏁 Script executed:
find web/pgadmin/browser/server_groups/servers/roles -name "*.py" | head -20Repository: pgadmin-org/pgadmin4
Length of output: 786
🏁 Script executed:
rg -n "rolvaliduntil" web/pgadmin/browser/server_groups/servers/roles/ -A 3 -B 3Repository: pgadmin-org/pgadmin4
Length of output: 26063
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql | head -60Repository: pgadmin-org/pgadmin4
Length of output: 2631
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql | head -60Repository: pgadmin-org/pgadmin4
Length of output: 2805
🏁 Script executed:
grep -n "rolvaliduntil" web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql | head -3Repository: pgadmin-org/pgadmin4
Length of output: 214
🏁 Script executed:
rg -n "rolvaliduntil.*empty|rolvaliduntil.*''" web/pgadmin/browser/server_groups/servers/roles/tests/ -iRepository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
sed -n '164,181p' web/pgadmin/browser/server_groups/servers/roles/__init__.pyRepository: pgadmin-org/pgadmin4
Length of output: 820
Align rolvaliduntil check in update.sql with create.sql pattern for consistency and clarity.
The templates show an inconsistency: create.sql uses {% if data.rolvaliduntil and data.rolvaliduntil is not none %} (explicit), while update.sql uses {% if data.rolvaliduntil %} (implicit truthiness). Since empty strings are falsy, an empty rolvaliduntil in update.sql will incorrectly generate VALID UNTIL 'infinity'. Use the explicit is not none check from create.sql in both update templates (lines 43-46 in 16_plus and default versions) to match the clearer, more defensive pattern.
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql`
around lines 3 - 10, In update.sql replace the implicit truthiness check "{% if
data.rolvaliduntil %}" with the explicit null check used in create.sql (e.g.,
"{% if data.rolvaliduntil and data.rolvaliduntil is not none %}" or at minimum
"{% if data.rolvaliduntil is not none %}") so that an empty string does not get
treated as a valid value; apply the same change to both the 16_plus and default
update.sql templates to mirror create.sql and ensure rolvaliduntil is handled
consistently.
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py`:
- Around line 848-861: In transform(), the re.search() calls used to parse role
strings can return None and are not checked; update the transform() logic to
verify the regex match before accessing groups: after calling re.search(...)
assign to a variable (e.g., match), if match is None skip/handle the malformed
entry (e.g., continue or log and skip) and only then use match.group(...) to
build the dict appended to res; apply this for both the pre-160000 and else
branches that reference match.group(1..4).
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql`:
- Line 18: The CASE that emits the CONNECTION LIMIT currently uses "rolconnlimit
> 0" which omits the valid "CONNECTION LIMIT 0"; update the condition in the
CASE expression that references rolconnlimit (in the roles/sql template) to
check "rolconnlimit != -1" instead (matching the pattern used in create.sql and
update.sql) so that 0 is included and only -1 (unlimited) is excluded.
♻️ Duplicate comments (8)
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql (3)
43-46: Consider aligningrolvaliduntilcheck with create.sql for consistency.The check
{% if data.rolvaliduntil %}treats empty strings as falsy, generatingVALID UNTIL 'infinity'. The create.sql uses a more explicit pattern. While this is minor, aligning the templates would improve maintainability.
56-61: Guard-render mismatch can produce invalid SQL if keys are missing.The condition
'admin' in item or 'inherit' in item or 'set' in itemallows entry if ANY key exists, but then unconditionally renders all three values ({{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}). If validation is bypassed or data is malformed, this produces broken SQL likeWITH ADMIN true, INHERIT , SET.Suggested fix using defaults
{% if data.rolmembership_list and data.rolmembership_list|length > 0 %} {% for item in data.rolmembership_list %} -GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; +GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin|default(false) }}, INHERIT {{ item.inherit|default(true) }}, SET {{ item.set|default(true) }}{% endif %}; {% endfor %} {% endif %}
114-118: Same guard-render mismatch exists for rol_members_list.Apply the same defensive fix with Jinja defaults here for consistency.
Suggested fix
{% for item in data.rol_members_list %} -GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }} {% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; +GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin|default(false) }}, INHERIT {{ item.inherit|default(true) }}, SET {{ item.set|default(true) }}{% endif %}; {% endfor %}web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sql (2)
11-15: LEFT JOIN can produce NULL entries in rolmembership array; seclabels ordering remains nondeterministic.The LEFT JOIN on line 12 can yield
rm.rolname = NULLwhen the role no longer exists, causing the concatenation to produce NULL entries in the array. Additionally, thearray_aggon line 15 lacksORDER BY, making results nondeterministic across executions.Suggested fix
ARRAY( SELECT CASE WHEN am.admin_option THEN '1' ELSE '0' END || CASE WHEN am.inherit_option THEN '1' ELSE '0' END || CASE WHEN am.set_option THEN '1' ELSE '0' END || rm.rolname FROM (SELECT * FROM pg_catalog.pg_auth_members WHERE member = r.oid) am - LEFT JOIN pg_catalog.pg_roles rm ON (rm.oid = am.roleid) + JOIN pg_catalog.pg_roles rm ON (rm.oid = am.roleid) ORDER BY rm.rolname ) AS rolmembership, - (SELECT pg_catalog.array_agg(provider || '=' || label) FROM pg_catalog.pg_shseclabel sl1 WHERE sl1.objoid=r.oid) AS seclabels + (SELECT pg_catalog.array_agg(provider || '=' || label ORDER BY provider, label) FROM pg_catalog.pg_shseclabel sl1 WHERE sl1.objoid=r.oid) AS seclabels
17-31: Add catalog qualification and ORDER BY for deterministic rolmembers output.The inner query references
pg_rolesandpg_auth_memberswithoutpg_catalog.qualification, and the outer ARRAY SELECT lacks anORDER BYclause, making results nondeterministic.Suggested fix
,ARRAY( SELECT CASE WHEN pg.admin_option THEN '1' ELSE '0' END || CASE WHEN pg.inherit_option THEN '1' ELSE '0' END || CASE WHEN pg.set_option THEN '1' ELSE '0' END || pg.usename FROM - (SELECT pg_roles.rolname AS usename, + (SELECT pg_catalog.pg_roles.rolname AS usename, pg_auth_members.admin_option AS admin_option, pg_auth_members.inherit_option AS inherit_option, pg_auth_members.set_option AS set_option - FROM pg_roles - JOIN pg_auth_members ON pg_roles.oid=pg_auth_members.member AND pg_auth_members.roleid={{ rid|qtLiteral(conn) }}::oid) pg + FROM pg_catalog.pg_roles + JOIN pg_catalog.pg_auth_members ON pg_catalog.pg_roles.oid=pg_catalog.pg_auth_members.member AND pg_catalog.pg_auth_members.roleid={{ rid|qtLiteral(conn) }}::oid) pg + ORDER BY pg.usename ) rolmembersweb/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js (1)
61-67: Add defensive checks for potentially undefined member properties.When
serverVersion >= 160000, the code directly calls.toString()onmember.admin,member.inherit, andmember.set. If these properties are undefined or null (e.g., from legacy data migration or API inconsistencies), this will throw a TypeError.Suggested defensive fix
- const serverVersion = this.nodeInfo && this.nodeInfo.server && this.nodeInfo.server.version || 0; + const serverVersion = this.nodeInfo?.server?.version ?? 0; rawData.forEach(member => { - let badges = serverVersion >= 160000 ? ` [WITH ADMIN ${member.admin.toString().toUpperCase()}, INHERIT ${member.inherit.toString().toUpperCase()}, SET ${member.set.toString().toUpperCase()}]` : member.admin ? ' [WITH ADMIN OPTION]' : ''; + let badges = ''; + if (serverVersion >= 160000) { + const admin = (member.admin ?? false).toString().toUpperCase(); + const inherit = (member.inherit ?? true).toString().toUpperCase(); + const set = (member.set ?? true).toString().toUpperCase(); + badges = ` [WITH ADMIN ${admin}, INHERIT ${inherit}, SET ${set}]`; + } else { + badges = member.admin ? ' [WITH ADMIN OPTION]' : ''; + }web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
218-236: Aliasing issue: list assignments are mutated via.extend().The direct assignment at lines 219 and 221 creates references to the original input data, which are then mutated by
.extend()calls at lines 224-225 and 231-232. This can cause unexpected side effects on the caller's data.As noted in a prior review, wrap these with
list()for defensive copies:if id == -1: - data['rolmembership_list'] = data['rolmembership'] + data['rolmembership_list'] = list(data['rolmembership']) else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', []))
821-840: Regex search result not guarded againstNone→ potentialAttributeError.The
re.search()calls at lines 827 and 833 returnNoneif the pattern doesn't match, causing.group()to raiseAttributeError. This was flagged in a prior review.Add guards as previously suggested:
if self.manager.version < 160000: - role = re.search(r'([01])(.+)', role) + m = re.match(r'^([01])(.+)$', role or '') + if not m: + continue rolmembers.append({ - 'role': role.group(2), - 'admin': True if role.group(1) == '1' else False + 'role': m.group(2), + 'admin': m.group(1) == '1' })
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
210-216: Consider extracting_part_dict_listas a module-level or class-level helper.The same helper function
_part_dict_listis defined identically in both_process_rolemembership(line 210) and_process_rolmembers(line 243). This violates DRY principle and increases maintenance burden.♻️ Proposed refactor
Extract to a static method or module-level function:
+ `@staticmethod` + def _part_dict_list(dict_list, condition): + """Filter dict_list by condition, returning matching dicts.""" + return [d for d in dict_list if condition(d)] + def _process_rolemembership(self, id, data): """ Process the input rolemembership list to appropriate keys :param id: id of role :param data: input role data """ - def _part_dict_list(dict_list, condition): - ret_val = [] - for d in dict_list: - if condition(d): - ret_val.append(d) - - return ret_val - if id == -1: data['rolmembership_list'] = data['rolmembership'] else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', [])) # ... rest of method using self._part_dict_listAlso applies to: 243-249
280-301: Error message includesinherit/setfields but validation is version-dependent.The error message (lines 280-291) shows
inheritandsetfields as required, but validation (lines 293-301) only enforces them for PostgreSQL 16+. This could confuse users on older PostgreSQL versions.Consider making the error message version-aware to match the actual validation requirements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/en_US/images/role_membership.pngis excluded by!**/*.pngdocs/en_US/images/role_sql.pngis excluded by!**/*.png
📒 Files selected for processing (42)
docs/en_US/role_dialog.rstweb/pgadmin/browser/server_groups/servers/roles/__init__.pyweb/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.jsweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.jsonweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/test.jsonweb/pgadmin/browser/server_groups/servers/static/js/membership.ui.jsweb/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsxweb/pgadmin/static/js/SchemaView/common.jsweb/pgadmin/static/js/SchemaView/options/index.js
🚧 Files skipped from review as they are similar to previous changes (26)
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sql
- web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msql
- web/pgadmin/static/js/SchemaView/common.js
- web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msql
- web/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsx
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msql
- docs/en_US/role_dialog.rst
- web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sql
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/static/js/SchemaView/options/index.js (1)
web/pgadmin/static/js/SchemaView/common.js (4)
isModeSupportedByField(15-17)isModeSupportedByField(15-17)isFieldSupportedByPgVersion(19-34)isFieldSupportedByPgVersion(19-34)
🔇 Additional comments (17)
web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msql (1)
1-7: AI summary is inconsistent with actual file content.The AI summary claims this file adds "four GRANT statements" including
WITH INHERIT OPTIONandWITH SET OPTION, but the actual file contains 2 GRANT statements and 2 REVOKE statements, all using onlyWITH ADMIN OPTION. This is correct behavior since this file is in thedefault/folder (pre-PostgreSQL 16), and INHERIT/SET options are PostgreSQL 16+ features expected in the16_plus/folder.The SQL syntax is correct, and the special character escaping in the role name using
""to escape embedded double quotes is properly handled.web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msql (1)
10-19: LGTM!The test file correctly exercises role membership scenarios including:
- Granting a role TO the complex-named role (lines 10, 12)
- Granting the complex-named role TO other roles (lines 17, 19)
- Both WITH ADMIN OPTION and without variants
The special character escaping in the role name (
"Role2_$%{}[]()&*^!@""'/#"`) is properly handled with double-quote escaping.web/pgadmin/static/js/SchemaView/options/index.js (2)
18-21: Good refactor to centralize version-gating logic.The new imports properly leverage the shared helper functions from
../commonfor consistent field visibility evaluation across the codebase.
48-57: LGTM!The
VISIBLEevaluator now correctly composes three checks:
isModeSupportedByField- validates field mode compatibilityisFieldSupportedByPgVersion- validates server type and version constraints- Original
field[VISIBLE]evaluationThis ensures fields with
min_version,max_version, orserver_typeconstraints are properly hidden when the connected server doesn't meet the requirements. The short-circuit evaluation order is correct.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sql (1)
1-18: LGTM!This test file provides good coverage for PostgreSQL 16+ role features:
- LOGIN capability with various role attributes
- Database-specific configuration (
SET application_name)- Role comments
- Proper escaping of special characters in role names
The use of
<PASSWORD>and<TIMESTAMPTZ_1>placeholders follows the test framework's parameterization pattern.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msql (1)
1-3: LGTM!Good test coverage for membership revocation scenarios:
- Revoking a role FROM the complex-named role (line 1)
- Revoking the complex-named role FROM another role (line 3)
This properly tests the bidirectional REVOKE operations that complement the GRANT tests in other files.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sql (1)
1-13: LGTM!The test file correctly covers PostgreSQL 16+ role creation with:
- Extensive special character handling in the role name
- Explicit role attribute settings (NOLOGIN, NOSUPERUSER, INHERIT, etc.)
- Role comments
The commented DROP statement (line 2) serves as documentation showing how to clean up the role if needed.
web/pgadmin/browser/server_groups/servers/roles/tests/default/test.json (2)
45-86: LGTM! Test role creation scenarios are well-structured.The new test roles (Role 3 and Role 4) follow the same pattern as existing roles and provide adequate test coverage for the
rolmembersfunctionality being added alongsiderolmembership.
148-149: LGTM! Alter scenarios comprehensively test membership operations.The test data properly covers add, change, and delete operations for both
rolmembershipandrolmembers, ensuring the new list-based structure is exercised through various update paths.Also applies to: 163-165, 179-181, 197-202
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql (2)
50-69: LGTM! List-based membership handling is correctly implemented.The template properly iterates over
rolmembership_revoked_list,rolmembership_list, androlmembership_revoked_adminsto generate REVOKE/GRANT statements. The identifier quoting viaqtIdentis applied correctly, and the admin option revocation logic appropriately checks for'admin' in item and not item.admin.
116-135: LGTM! Role members handling mirrors the membership pattern correctly.The
rol_members_*lists follow the same structure asrolmembership_*, with the grant/revoke direction correctly reversed (grantingrolnameTOitem.roleinstead of vice versa).web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js (1)
180-183: LGTM! Arrow function wrappers correctly preservethiscontext.The change from direct method reference to arrow function wrapper
(raw) => obj.memberDataFormatter(raw)ensures properthisbinding when the formatter is invoked.Also applies to: 201-204
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql (1)
31-56: LGTM! Well-structured GRANT statement generation for PostgreSQL 16+.The conditional handling of ADMIN OPTION, INHERIT OPTION, and SET OPTION with proper comma placement is correctly implemented. Grouping by all three flags ensures grants with identical options are consolidated into single statements.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msql (1)
1-9: LGTM! Good test coverage for special character handling in role names.The test correctly validates SQL generation for role names containing special characters including quotes, backslashes, and various symbols. This ensures proper identifier escaping in the templating system.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sql (1)
1-21: LGTM! Comprehensive test for role alteration with PostgreSQL 16+ membership options.The test correctly validates the expected SQL output including:
- Role creation with attributes and password/validity placeholders
- GRANT statements both with and without the new WITH options (ADMIN, INHERIT, SET)
- Database-specific settings and comments
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.json (2)
148-149: LGTM! Good coverage of membership options with admin, inherit, and set flags.The test data correctly exercises the new PostgreSQL 16+ membership options including added and changed membership entries with all three boolean flags (admin, inherit, set).
172-209: LGTM! Comprehensive alter role options tests.The test scenarios 2-4 effectively cover:
- Changed memberships (options 2)
- Deleted memberships (options 3)
- Combined add + change operations (options 4)
This provides good coverage for the membership management workflows.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if self.manager.version < 160000: | ||
| role = re.search(r'([01])(.+)', role) | ||
| res.append({ | ||
| 'role': role.group(2), | ||
| 'admin': True if role.group(1) == '1' else False | ||
| }) | ||
| else: | ||
| role = re.search(r'([01])([01])([01])(.+)', role) | ||
| res.append({ | ||
| 'role': role.group(4), | ||
| 'admin': True if role.group(1) == '1' else False, | ||
| 'inherit': True if role.group(2) == '1' else False, | ||
| 'set': True if role.group(3) == '1' else False | ||
| }) |
There was a problem hiding this comment.
Same regex None handling issue in transform() method.
The re.search() calls at lines 849 and 855 have the same vulnerability as _set_rolemembers - no guards against None results when the pattern doesn't match.
🐛 Proposed fix
for role in roles:
if self.manager.version < 160000:
- role = re.search(r'([01])(.+)', role)
+ m = re.match(r'^([01])(.+)$', role or '')
+ if not m:
+ continue
res.append({
- 'role': role.group(2),
- 'admin': True if role.group(1) == '1' else False
+ 'role': m.group(2),
+ 'admin': m.group(1) == '1'
})
else:
- role = re.search(r'([01])([01])([01])(.+)', role)
+ m = re.match(r'^([01])([01])([01])(.+)$', role or '')
+ if not m:
+ continue
res.append({
- 'role': role.group(4),
- 'admin': True if role.group(1) == '1' else False,
- 'inherit': True if role.group(2) == '1' else False,
- 'set': True if role.group(3) == '1' else False
+ 'role': m.group(4),
+ 'admin': m.group(1) == '1',
+ 'inherit': m.group(2) == '1',
+ 'set': m.group(3) == '1'
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.manager.version < 160000: | |
| role = re.search(r'([01])(.+)', role) | |
| res.append({ | |
| 'role': role.group(2), | |
| 'admin': True if role.group(1) == '1' else False | |
| }) | |
| else: | |
| role = re.search(r'([01])([01])([01])(.+)', role) | |
| res.append({ | |
| 'role': role.group(4), | |
| 'admin': True if role.group(1) == '1' else False, | |
| 'inherit': True if role.group(2) == '1' else False, | |
| 'set': True if role.group(3) == '1' else False | |
| }) | |
| if self.manager.version < 160000: | |
| m = re.match(r'^([01])(.+)$', role or '') | |
| if not m: | |
| continue | |
| res.append({ | |
| 'role': m.group(2), | |
| 'admin': m.group(1) == '1' | |
| }) | |
| else: | |
| m = re.match(r'^([01])([01])([01])(.+)$', role or '') | |
| if not m: | |
| continue | |
| res.append({ | |
| 'role': m.group(4), | |
| 'admin': m.group(1) == '1', | |
| 'inherit': m.group(2) == '1', | |
| 'set': m.group(3) == '1' | |
| }) |
🤖 Prompt for AI Agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py` around lines 848
- 861, In transform(), the re.search() calls used to parse role strings can
return None and are not checked; update the transform() logic to verify the
regex match before accessing groups: after calling re.search(...) assign to a
variable (e.g., match), if match is None skip/handle the malformed entry (e.g.,
continue or log and skip) and only then use match.group(...) to build the dict
appended to res; apply this for both the pre-160000 and else branches that
reference match.group(1..4).
| -- PostgreSQL >= 9.1 | ||
| CASE WHEN rolreplication THEN 'REPLICATION' ELSE 'NOREPLICATION' END || E'\n ' || | ||
| CASE WHEN rolbypassrls THEN 'BYPASSRLS' ELSE 'NOBYPASSRLS' END || | ||
| CASE WHEN rolconnlimit > 0 THEN E'\n CONNECTION LIMIT ' || rolconnlimit ELSE '' END || |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL CONNECTION LIMIT 0 valid role setting semantics
💡 Result:
Short answer
- CONNECTION LIMIT 0 means “zero allowed concurrent connections” — the role (or database) is prevented from making new connections. [1][2]
- Superusers are still able to connect (so LIMIT 0 is commonly used to block non-superusers during maintenance). [3]
Sources
- PostgreSQL ALTER ROLE / CREATE ROLE docs (connection-limit semantics). [1]
- pg_roles catalog (rolconnlimit: -1 = no limit; otherwise the integer is the max concurrent connections). [2]
- Official mailing-list / DBA guidance and Q&A showing practical use of CONNECTION LIMIT 0 to block non-superuser connections during maintenance. [3]
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql | head -30Repository: pgadmin-org/pgadmin4
Length of output: 1754
🏁 Script executed:
cat -n web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql | grep -i "connlimit\|limit"Repository: pgadmin-org/pgadmin4
Length of output: 160
🏁 Script executed:
# Check if there are older PostgreSQL versions of this template to compare
find web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql -name "*.sql" -type f | head -10Repository: pgadmin-org/pgadmin4
Length of output: 950
🏁 Script executed:
grep -n "rolconnlimit" web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/sql.sqlRepository: pgadmin-org/pgadmin4
Length of output: 156
🏁 Script executed:
# Check if there are any tests or comments explaining the CONNECTION LIMIT logic
grep -r "CONNECTION LIMIT" web/pgadmin/browser/server_groups/servers/roles/ --include="*.py" --include="*.sql" -B 2 -A 2Repository: pgadmin-org/pgadmin4
Length of output: 9946
Fix rolconnlimit > 0 condition to include valid CONNECTION LIMIT 0 setting.
A CONNECTION LIMIT 0 is valid in PostgreSQL and prevents non-superuser connections. The condition rolconnlimit > 0 excludes this legitimate value, causing it to be omitted from the generated DDL. Change to rolconnlimit != -1 (where -1 means unlimited) to match the pattern used in create.sql and update.sql.
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql`
at line 18, The CASE that emits the CONNECTION LIMIT currently uses
"rolconnlimit > 0" which omits the valid "CONNECTION LIMIT 0"; update the
condition in the CASE expression that references rolconnlimit (in the roles/sql
template) to check "rolconnlimit != -1" instead (matching the pattern used in
create.sql and update.sql) so that 0 is included and only -1 (unlimited) is
excluded.
2 participants
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.